-
Notifications
You must be signed in to change notification settings - Fork 118
feat(network): Send product information to distinguish clients (with a colored name and different status) #1404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(network): Send product information to distinguish clients (with a colored name and different status) #1404
Conversation
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPICallbacks.cpp
Outdated
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPICallbacks.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Diplomacy.cpp
Outdated
Show resolved
Hide resolved
|
What is the current status of this change? I think this would be a nice change for the first release. |
The current implementation ought to be changed to make it more efficient / optimal, but that requires much more work. I did some of the work locally, but it didn't really work out iirc, so the PR is kind of stuck for now. |
9fb6a01 to
c4e3f1d
Compare
|
I've revisited this PR and was able to move away from the broadcast implementation. It feels better now. I put networking related changes in one commit and the color / status stuff in another. What sort of information do we want to exchange just to establish which clients are on a patched version? Maybe it's a good idea to send the commit hash as well. |
|
Short overview of the new implementation. Patch information is not broadcast and only sent when needed. Interacting with players in the lobby:
Interacting with game hosts in the lobby:
Interacting with players in the pre-match game:
Currently, players in the lobby do not have a way to know which players in a match are on a 'patched version' except the host. It doesn't seem necessary to me to have the host communicate this information, and it also breaks the current pattern where each client only sends data about itself. |
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Diplomacy.cpp
Outdated
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is going into the right direction but needs more polishing and wider scope.
Are the pictures of the first post still relevant?
The terminology of the network data needs changing from "Patch" to "Product", because "Patch" is a much more limited term than "Product" is.
The ProductInfo message needs to contain all relevant information to describe the Product that the remote player is using. A version number alone will not be enough to properly describe a Product. Think of 100 different products that the user could interact with in the lobby.
|
|
||
| void handlePatchInfo(Int messageType, UnsignedInt senderIP, UnicodeString gameName); | ||
| void handleGameRequestPatchInfo(LANMessage *msg, UnsignedInt senderIP); | ||
| void handleGameAcknowledgePatchInfo(LANMessage *msg, UnsignedInt senderIP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "Acknowledge" synonymous for replying to the request message? Perhaps call it "Response". And add it to the end of the name.
handleGamePatchInfoRequest
handleGamePatchInfoResponse
Even when that breaks the current EA naming pattern, it is a better naming pattern in my opinion.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "Acknowledge" synonymous for replying to the request message? Perhaps call it "Response".
Yes, 'Acknowledge' is a response to a patch info request.
Should the enum still contain 'ACKNOWLEDGE'? (e.g. MSG_GAME_ACKNOWLEDGE_PATCH_INFO)
There's also a bunch of comments where I'm currently using the term 'acknowledge'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked Chat what ACK refers to in Networking:
🧩 Definition
In networking, to acknowledge means:
To confirm receipt of data — that a packet, frame, or message has successfully arrived.
So when one device sends data to another, the receiver acknowledges it by sending back a message (an acknowledgment, or ACK) saying essentially:
“I got it — you can send the next one.”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ACK terminology only to confirm the receipt of a message, not for a response to a request. When the message is a request and in need of a response, then use the term "Response" for the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved away from 'acknowledge' now, except for in a few of the comments.
| struct | ||
| { | ||
| WideChar gameName[g_lanGameNameLength + 1]; | ||
| UnsignedInt patchVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Product Name and Author Name are missing. Probably also needs to send EXE and INI hash, because Product Name and Version are not necessarily unique and hashes need to be part of identifying product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I added all of these, but I'm not yet storing any of the data per player slot.
Renamed 'patch' to 'product' and added more information to the product information that's exchanged between clients.
Yes, the colors are still like that; the status text will be slightly different. My current idea is that it shows the Git short hash string there (there isn't much room for additional text, unfortunately).
Fair enough. Everything should say 'product' instead of 'patch' now.
I added a couple of stuff (e.g. exe and ini crc values), but I don't feel like this is final version yet. |
| UnsignedInt productVersion; | ||
| Char gitTagOrHash[33]; | ||
| WideChar productName[129]; | ||
| } ProductInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct is kind of work-in-progress for now, but eventually any string sizes should be named like in the other structs.
|
|
||
| void LANAPI::setProductInfoFromMessage(LANMessage *msg, GameSlot *slot) | ||
| { | ||
| GameSlot::ProductInfo productInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a constructor for GameSlot::ProductInfo? And if so, should it take all these fields as separate function parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is constructed in a few places only then POD should be fine.
With this PR it'll become clear which clients are on a patched version with this feature and which are not:
[SH]tag.LAN Lobby:

Pre-match / Game Options:

Status in match:

TODO: